Skip to content

ci: certify Stripe Authorize through test_ucs - #2086

Open
gopikrishna000 wants to merge 14 commits into
mainfrom
poc/test-ucs-stripe-authorize-ci
Open

ci: certify Stripe Authorize through test_ucs#2086
gopikrishna000 wants to merge 14 commits into
mainfrom
poc/test-ucs-stripe-authorize-ci

Conversation

@gopikrishna000

Copy link
Copy Markdown
Contributor

Summary

  • fail CI when connector credentials are unavailable in the test job
  • add a POC Stripe card Authorize certification step using the data-driven test_ucs runner
  • run the stable no3ds_auto_capture_credit_card scenario with report output

Why

This proves the connector_specs/scenario.json/test_ucs path can be used as a merge-blocking behavior certification gate, instead of only validating schema/spec declarations.

Notes

This POC is intentionally scoped to Stripe card Authorize. The next step is to derive changed connector names from CI and run the same command for each changed connector.

@gopikrishna000
gopikrishna000 requested a review from a team as a code owner August 10, 2026 13:24
gopikrishna000 and others added 4 commits August 10, 2026 19:28
…r_id

The new Stripe Authorize certification step in CI (test_ucs) failed for
two reasons, both pre-existing but never exercised until this PR ran a
real-credential grpcurl call against Stripe:

- scenario.json sent customer.phone_number as a bare string, but the
  proto types it as SecretString ({"value": "..."}), so grpcurl failed
  to parse the request entirely.
- The harness's standard "unset" placeholder connector_customer_id: ""
  flowed through as Some("") into PaymentFlowData.connector_customer,
  which Stripe's transformer forwards verbatim as customer="". Stripe
  rejects an explicit empty customer parameter outright. Added
  non_empty_connector_customer_id() and applied it at all 9 conversion
  sites in domain_types so every connector gets None instead of Some("").

Verified locally: test_ucs --connector stripe --suite
PaymentService/Authorize --scenario no3ds_auto_capture_credit_card
--skip-dependencies now PASSes against real Stripe test credentials.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@gopikrishna000
gopikrishna000 requested a review from a team as a code owner August 11, 2026 07:04
@XyneSpaces

Copy link
Copy Markdown

Review findings (aggregated; inline placement not available via this tooling):

[should-fix] .github/workflows/ci.yml:508
The credential-setup step now exit 1s if the decrypted creds file is missing, turning the previous graceful fallback into a hard failure for every RUN_TESTS=true run. S3/GPG misconfiguration or transient decryption failures will break the whole test job. Move the fail-fast behavior into the new certification step so the generic test job can still degrade safely.

[should-fix] .github/workflows/ci.yml:571-577
grpcurl is downloaded from a hard-coded GitHub release (1.9.3, linux_x86_64) with no checksum verification. This breaks on non-x86_64 runners and supplies no integrity guarantee. Pin via the runner's package manager or verify the archive SHA256 and gate on runner.arch.

[should-fix] .github/workflows/ci.yml:585-592
The Stripe certification run passes --skip-dependencies for a PaymentService/Authorize scenario; if the fixture relies on prerequisite context (e.g., a created customer), the certification may issue an invalid request or report a misleading pass/fail. Either remove --skip-dependencies or confirm no3ds_auto_capture_credit_card has no required prerequisites.

@XyneSpaces

Copy link
Copy Markdown

[should-fix] CI downloads grpcurl release tarball without checksum verification

The new "Install grpcurl" step in .github/workflows/ci.yml curls grpcurl_${GRPCURL_VERSION}_linux_x86_64.tar.gz directly from GitHub Releases and extracts it without verifying a SHA-256 or signature. Add a pinned checksum comparison (or use a verified GitHub Action) before extracting.

@XyneSpaces

Copy link
Copy Markdown

[should-fix] Nested credential loader selects first match with undefined ordering

extract_connector_block iterates obj.values() and returns the first nested object containing connector_account_details. If the credentials file has multiple environments, serde_json::Map's iteration order determines which account is used, which can run certification against the wrong environment. Explicitly resolve the intended environment key and fail if more than one candidate is present.

gopikrishna000 and others added 9 commits August 11, 2026 20:48
Replaces the hardcoded single Stripe certification step with a loop
over .github/test/certified_connectors.json, so onboarding a new
certified connector is a reviewed manifest edit instead of a copy-pasted
CI step. Certification now runs whenever core/proto/CI/shared-harness
code changes (any certified connector could be affected) or when a
connector-only PR touches a connector already in the manifest;
otherwise it's skipped to keep cost bounded as PR volume grows.

--skip-dependencies is no longer a trusted flag. The harness checks the
scenario's own request against the suite's context_map before honoring
a skip request: a target path is only treated as satisfied if the
scenario supplies its own value there, or if a sibling oneof variant
was chosen instead (e.g. payment_method.card over payment_method.token).
Anything else now fails with a explicit DependenciesRequired error
instead of silently skipping a dependency the scenario actually needs.

Also adds a bounded retry (3 attempts, 10s backoff) around each
certification call so a transient sandbox blip doesn't fail an
unrelated PR — verified there's no existing retry/timeout in
run-tests or the grpcurl invocation this would stack with.
Auto-applied by CI:
- cargo +nightly fmt --all
- make -C sdk generate (if applicable)
- make docs (if applicable)

This commit was automatically generated by GitHub Actions.
…re create_all_prerequisites! macro

The skip-dependency safety check read the raw, connector-agnostic base
scenario.json instead of the connector-merged request (base + connector_specs
override.json). It only gave the right answer for Stripe by coincidence,
since Stripe adds no override for the certified scenario; any connector whose
override changes a context_map-relevant field would get a wrong answer.
Now goes through load_effective_scenario_for_connector, matching what
actually gets sent. Added a regression test using adyen's override.json
(the one connector confirmed to patch a context_map-relevant field) to
prove the merge is actually exercised.

check_connector_specs.rs previously skipped Phase 2's specs.json coverage
check entirely for any connector without a create_all_prerequisites! macro,
never counting it as a failure. Only razorpay and razorpayv2 lack the
macro (confirmed by full sweep of all 101 connector files) — they predate
this requirement and wire up flows via hand-written impl blocks with no
structured, parseable declaration. Named them as an explicit, reviewed
legacy exception list; every other connector missing the macro now fails
the check instead of silently passing.

Also narrowed non_empty_connector_customer_id's doc comment: it's proven
necessary (via a real Stripe sandbox call) only for the Authorize call site;
the other 8 call sites reuse it by analogy, unverified, and one of them
(CustomerServiceGetRequest, a lookup-by-ID path) may not even want the same
behavior. Flagged so it isn't extended to new fields without the same kind
of live-connector proof.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every connector PR sampled this session (Moneris, Citigate, GrabPay
merged, AsiaPay) shipped with only fixture-based specs.json coverage —
zero proof any flow actually works against the connector's real sandbox.
certified_connectors.json alone doesn't close this gap: it's an opt-in,
reviewed allowlist that nobody adds a brand-new connector to on day one.

Adds detection of newly-introduced connectors (connector_specs/<name>/
didn't exist at the PR's base commit) and a self-certify step, separate
from the existing cert-gate: it fires only on that connector's own
introducing PR, never on unrelated core/proto changes, so it can't
inflate CI cost or blast radius for PRs that don't touch a new connector.

The connector author declares readiness in their own specs.json via a
self_certify block (suite, scenario, skip_dependencies, has_live_creds),
since only they know whether sandbox creds have been provisioned yet.
has_live_creds: true actually runs the call via run-tests and must PASS.
has_live_creds: false is not a silent pass — it's a visible, named
::warning:: so "merged without live proof" stays tracked instead of
disappearing, distinct from a real failure.

certified_connectors.json's schema note now documents the relationship:
it's the ongoing-regression tier (re-verifies an already-proven connector
on every future relevant change), while self_certify is the one-time,
per-connector proof gate on that connector's own introducing PR.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e/HandleEvent

Webhook coverage was completely unenforced: only 5 of ~100 connectors
have webhook_payload.json even though more declare EventService/HandleEvent
in supported_suites. Detection is deterministic — no naming-convention
heuristics, no new schema field — the connector's own specs.json
supported_suites list already says whether it claims webhook support,
using the same suite-name vocabulary this checker already uses
everywhere else.

Confirmed one existing connector (phonepe) declares EventService/HandleEvent
with no webhook_payload.json — this hard-fails CI on main until it's added.
Not fixed here: writing a real payload requires phonepe's actual webhook
signature/event contract, which shouldn't be fabricated. Flagging this as
a known consequence of merging this PR, not something silently absorbed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e/proto changes

cert-gate previously fired on any rust-core/proto/CI change, taxing every
unrelated PR (e.g. a Citigate PR touching router_data.rs for its own field)
with a real Stripe API call it has nothing to do with. A full-fleet,
all-connector, real-sandbox regression run already exists independently —
Jenkins "Generate Test Report" job (hyperswitch-infra,
connector-service-report-pipeline branch,
.jenkins/connector-service/generate-test-report.Jenkinsfile), using
browser-automation-engine/ and run-tests --all-connectors by default — and
is the right place for "did this core/proto change break some connector,"
gating deployment/nightly tags rather than individual PRs.

cert-gate now fires only when a certified connector's own files change
(certified-connectors-touched), including a fix to certified-connectors
detection: it previously only checked connector_specs/connector-integration
file changes, missing the case where certified_connectors.json itself is
edited to add a new certified connector — that PR now correctly triggers
its own first certification run instead of silently waiting for some
unrelated future PR to touch the newly-certified connector's files.

Also extended the stripe path-filter to connectors/stripe/** and
connector_specs/stripe/** (previously only stripe.rs and
stripe/transformers.rs), so an override.json or specs.json change for
Stripe re-triggers certification too.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nature support

The new webhook coverage check (previous commit) fails CI on main because
phonepe declares EventService/HandleEvent in specs.json but had no
webhook_payload.json fixture. This PR is what makes that check fail, so
this PR fixes it rather than leaving it for someone else.

Added crates/internal/integration-tests/src/connector_specs/phonepe/webhook_payload.json
with payment_success/payment_failed/payment_pending/invalid_signature
scenarios, matching the real contract read straight from
connectors/phonepe.rs's IncomingWebhook impl and transformers.rs's
PhonepeWebhookRequest/PhonepeWebhookPayload structs — not fabricated:
outer body is {"response": "<base64>"}, inner payload has
merchantId/merchantTransactionId/transactionId/amount/state/responseCode/
paymentInstrument, and state values map to PAYMENT_SUCCESS/PAYMENT_PENDING/
PAYMENT_ERROR etc. exactly as map_phonepe_webhook_state_to_event_type reads
them.

PhonePe's signature (X-VERIFY: sha256(inner_base64_body + api_path +
salt_key) + "###" + key_index, from compute_phonepe_webhook_checksum) needed
two things the generic webhook-signature test harness didn't support:
hashing the *inner* base64 field instead of the outer body, and extra
context (api_path, key_index) beyond payload+secret+timestamp. Rather than
special-case this in the caller, extended webhook_signatures.rs's existing
per-connector match dispatch (same pattern already used for
stripe/adyen/authorizedotnet/paypal) with a SignatureContext parameter and
a generate_phonepe_signature arm, and added two small connector-keyed
helpers in connector_override/mod.rs (signature_payload_bytes,
signature_context) so apply_webhook_payload_overrides itself never branches
on connector name — a future connector needing similar treatment adds a
match arm in each, not new branching logic.

api_path is fixture-controlled (request_details.uri), not a value that
needed discovering: nothing in this repo hardcodes phonepe's production
webhook receive path, and the fixture is the only source of truth the test
harness consults, so the same literal just needs to appear consistently on
both the request and the signature computation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…es, require no_creds_reason

Three real gaps in the new-connector proof requirement:

1. self_certify was a single object — only Authorize (or whichever one
   scenario) could ever get proven, even for a connector implementing
   6 flows with multiple payment methods. Now verified_scenarios is an
   array; every real scenario a connector wants proven gets its own entry.

2. "self_certify" reads as self-graded/unreliable, and risked being
   confused with certified_connectors.json's different, ongoing-regression
   meaning of "certified." Renamed to verified_scenarios — it's a running
   record of what's actually been proven, not a one-time gate; the array is
   expected to grow across later PRs as new payment methods/flows are added,
   not just filled in once at introduction.

3. has_live_creds: false was accepted with zero explanation, and only
   surfaced as a CI ::warning:: — easy to miss, and not visible in the PR
   diff a reviewer is actually looking at. no_creds_reason is now mandatory
   whenever has_live_creds is false, living in the same specs.json file the
   reviewer already reviews, not buried in an Actions log tab.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Auto-applied by CI:
- cargo +nightly fmt --all
- make -C sdk generate (if applicable)
- make docs (if applicable)

This commit was automatically generated by GitHub Actions.
@AmitsinghTanwar007

Copy link
Copy Markdown
Contributor

Review: Test trigger scope needs rethinking

The current approach intentionally skips certification when core/proto/shared files are touched:

"Firing this per-PR gate on every core/proto change would tax every unrelated PR"

I think this is backwards — core/proto changes are the most dangerous because they can silently break multiple connectors at once. A connector-specific change has limited blast radius, but a change to router_data.rs, domain_types, external-services, or proto definitions can break every connector.

Suggested approach (3 tiers):

Tier 1 — Core/Proto changes touched
→ Run tests for all certified/hot/prod connectors. These are high-value connectors you can't afford to break silently. Yes it adds CI time, but that's the point — catching regressions before merge.

Tier 2 — Connector-specific files touched
→ Run tests for that connector only. If someone changes stripe/transformers.rs, test Stripe.

Tier 3 — New connector added
→ Run its declared verified_scenarios as a one-time proof gate (this part is already in the PR).

The "tax" of running a few sandbox calls with retries on core changes is minimal compared to the cost of shipping a broken core change that silently breaks production connectors. The Jenkins nightly job catches it eventually, but by then the damage is already in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants